Fix zoom out not persisting while switching between editor and code editor #65932
Conversation
madhusudhand
added
[Type] Bug
|
Size Change: +894 B (+0.05%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 240932e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11230020780
|
madhusudhand
force-pushed
the
fix-zoomout-switching-editor
branch
from
240932e to
ab13653
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Looks like it is bring back the issue fixed in #64478 after switching to code editor and back to editor. This is because the scale calculation now directly depends on containerWidth (which is variable value) I have put this condition, it works for code editor switching (because the container width doesn't change), but fails when switching to different template. May be we have to preserve the previous container width outsize of component context? |
|
I would suggest that we disable zoom out (scaling and mode) when we enter code editing - the bug now is that the mode is still on, the scaling is off. Let's approach like all off because swithching to code is a sort of reset in terms of UI. Ideally we'll preserve the satate, but that is an enhancement. To fix the bug just set it all to off when switching to code. |
madhusudhand
force-pushed
the
fix-zoomout-switching-editor
branch
from
36a5afb to
990a939
Compare
Makes sense. Editor now dispatches reset action in cleanup step (when component is unmounted). |
| _src, | ||
| () => { | ||
| URL.revokeObjectURL( _src ); | ||
| resetZoomLevel(); |
There was a problem hiding this comment.
Should we reset both zoom and the mode? Check out the useZoomOut hook for all the stuff that is related.
There was a problem hiding this comment.
Yes, thanks for pointing. Included __unstableSetEditorMode('edit').
There was a problem hiding this comment.
Can someone explain why we're resetting zoom-out in the iframe component? That feels like a random place.
There was a problem hiding this comment.
I thought iframe to be better place because It is handling all zoom-out related calculations when component is mounted + on changes, and cleanup is naturally can go in there when it is unmounted.
There was a problem hiding this comment.
useZoomOut seems to be breaking with this changes (e.g. clicking on browse styles under global styles) triggers zoom-out and exits. This is likely because the hook triggered twice in dev mode.
There was a problem hiding this comment.
I think what's not clear to me is who is responsible for enabling/disabling zoom-out. Is it the "block editor" itself or is it something that the implementation (a button in the UI of the editor ... does).
Switching to "code editor" is not a "block editor" thing, so sure the iframe is destroyed when you move to block editor but that doesn't mean you moved to "code editor". Maybe it's a "preview iframe". So you just wanted to render something else temporarily...
There was a problem hiding this comment.
It's both. The design direction indicated a requirement for a manual toggle button. However, there are also scenarios where we want to be "intelligent" and enter/exit room out for certain actions.
I feel Zoom Out is a fairly opinionated state so we shouldn't leave it active if the users moves to do something else.
Switching to "code editor" is not a "block editor" thing, so sure the iframe is destroyed when you move to block editor but that doesn't mean you moved to "code editor". Maybe it's a "preview iframe". So you just wanted to render something else temporarily...
I'm sorry but I wasn't fully able to follow what you meant here...
There was a problem hiding this comment.
I'm sorry but I wasn't fully able to follow what you meant here...
The iFrame is a component that can be used in several context to render "block content". It's shouldn't be related to zoom-out (aside receiving a "scale" prop to adapt).
Also block-editor has zoom-out state but doesn't have "code editor" state. So if we want to exit zoom-out when we go into the code editor, it should be done at the highest common level (where the code editor state lives)
There was a problem hiding this comment.
Thanks for elaborating 👍
My understanding of the code in this PR is that the "zoom out mode" and "zoom level" will be reset when the iframe component is unmounted. This happens as part of a cleanup operation.
Your concern is that this is a blanket solution which will now apply each time the iframe is unmounted even in context's outside of the WordPress Editor implementation (this is because @wordpress/block-editor can be used to make an standalone editor).
What you are recommending is that we act specifically to exit zoom out where necessary based on the state that owns the action.
For example:
Requirement: exit Zoom Out when switching in/out of "Code Editor" mode.
Solution: add necessary code to exit Zoom Out to the switchEditorMode action.
Requirement: exit Zoom Out when switching between posts / templates in the Editor.
Solution: add necessary code to exit Zoom Out to setEditedEntity or perhaps useInitEditedEntityFromURL?
That seems logical. Rather than implement a blanket solution, allowing the consuming Editor instance to control when to exit Zoom Out rather than forcing on all implementations of block-editor.
There was a problem hiding this comment.
Yes, thanks for expanding. "unmounting the iframe" seems too broad. As you can unmount the iframe in block previews too when you render and show them... you could argue block previews have their own "block editor state" which is true, but the point here is that the iframe is about rendering the blocks, not controlling which state we're in.
getdave
changed the title
There was a problem hiding this comment.
Thanks for ongoing work here.
Tested and this now resets the Zoom when toggling Visual -> Code and back.
When moving between Pages Zoom Out is still persisted. I think we should be consistent with Templates and just reset it as soon as you change the post type / post ID.
Do you agree?
Screen.Capture.on.2024-10-11.at.11-10-01.1.mp4
I think you are referring to switching pages from command bar (screenshot doesn't load for me) In cases such as code editor, switching to other templates, pages etc from data views, user is moved out of editor and presented with something else and so zoom-out isn't valid in that context. (Hence adopted reset on exit of the editor) Where as switching a page from command bar, user stays in the current view of the editor. |
|
I updated the comment above to include the screencapture. I think we should exit zoom out when:
This PR is 99% of the way there. |
madhusudhand
force-pushed
the
fix-zoomout-switching-editor
branch
from
e630335 to
aa24cf9
Compare
|
@madhusudhand I think this needs changes based on this discussion. Do you think you could get these resolved by Friday? That will allow us to ensure there is time to get this merged by Monday in time for WP 6.7 RC1 🙏 |
madhusudhand
force-pushed
the
fix-zoomout-switching-editor
branch
from
aa24cf9 to
f82d9b9
Compare
|
@getdave Moved the logic to TextEditor to reset zoom-out mode, instead of resetting in iframe. Also, with latest trunk, switching templates or anything in data views, preserves the zoom-out. I think, the new behavior is intentional. I am excluding it from the current PR since it is no longer a bug. We can iterate it in a separate PR if it doesn't feel right. |
|
I tried to test this by:
Is this a playground issue where the patch is not being applied correctly? |
|
In response to @yani-:
The patch seems to have applied correctly 👍🏻 That is the expected behavior as suggested in #65932 (comment). Thanks for testing! @madhusudhand and @getdave, I'd also like to confirm that the patch works as expected (since #65932 (comment)). Test ReportEnvironment
Actual Results
Screenshot
|
There was a problem hiding this comment.
Thanks for addressing the feedback on this one @madhusudhand 👍
I've taken it for a spin and it is working for me also.
✅ Switching to the Code Editor resets zoom out correctly
✅ If zoom out is toggled back on it is honored when leaving the Code Editor
✅ Zoom out is still maintained while switching between templates etc.
The PR appears to be in line with the discussion and feedback received so far.
It might help others reviewing or trying to debug in the future if the PR title and description was updated to reflect the current state of this PR.
That minor nit aside, the only odd behaviour I noticed was the ability to toggle on zoom out in a context where it shouldn't apply. This is the same currently for the Style Book and Global Styles Revisions. I think it's fine to address that via a follow-up and the related issue: #66127.
If it helps land this fix for 6.7, I'm happy to give this a green light but feel free to seek a second approval from those that have been more involved up to this point.
Screen.Recording.2024-10-18.at.11.31.37.am.mp4
madhusudhand
changed the title
draganescu
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
…ditor (#65932) * reset zoomLevel on component unmount * conditionally reset zoom level * revert iframe changes and reset zoomLevel in the text-editor Co-authored-by: madhusudhand <madhudollu@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: yani- <yaniiliev@git.wordpress.org> Co-authored-by: ironprogrammer <ironprogrammer@git.wordpress.org> Co-authored-by: simison <simison@git.wordpress.org> Co-authored-by: PARTHVATALIYA <parthvataliya@git.wordpress.org>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 55f9895 |
|
Thanks for all the work here and reviews everyone 👍 |
…ditor (WordPress#65932) * reset zoomLevel on component unmount * conditionally reset zoom level * revert iframe changes and reset zoomLevel in the text-editor Co-authored-by: madhusudhand <madhudollu@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: getdave <get_dave@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: yani- <yaniiliev@git.wordpress.org> Co-authored-by: ironprogrammer <ironprogrammer@git.wordpress.org> Co-authored-by: simison <simison@git.wordpress.org> Co-authored-by: PARTHVATALIYA <parthvataliya@git.wordpress.org>
What?
This fixes the issue where zoom out isn't persisting while switching between
Closes: #65783
Why?
When the editor is switched, component gets destroyed and the value of
prevContainerWidthRef.currentis set to undefined.The hook doesn't set the value because it is in ZoomedOut mode.
Testing Instructions
Screenshots or screencast